Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The exception can misclassify malformed non-probe configurations and bypass channel validation.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Allows IPC4 probe initialization with a zeroed base configuration while retaining channel-count validation.
Changes:
- Detects zeroed base configurations.
- Exempts them from channel-count validation.
| File | Description |
|---|---|
src/audio/module_adapter/module_adapter_ipc4.c |
Adds the probe configuration exception. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The channel check introduced in commit: cdb7f91 ("audio: module_adapter: bound host channels_count at ipc4 init") does not let probes module through. Add extra condition to allows probes that relies on ipc4 probes implementation on Linux ASoC side, that initializes the whole base config as zero for probes module. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
PR 11260: test resultsRun date: 2026-10-09 09:11 UTC Tested commit: cf67fd40bd30a06860159e7dc11a74477c449a68
|
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Change looks good and needed, but wondering whether we could simplify check... see inline
| dst->base_cfg.audio_fmt.channels_count > PLATFORM_MAX_CHANNELS) { | ||
| if ((dst->base_cfg.audio_fmt.channels_count == 0 || | ||
| dst->base_cfg.audio_fmt.channels_count > PLATFORM_MAX_CHANNELS) && | ||
| !is_probe_module(&dst->base_cfg)) { |
There was a problem hiding this comment.
Could we just drop the check for channel count equal? Checking for a full-zero base config seems a bit hackish and if the original check is to prevent out-of-bounds access, just passing zero channel count would seem to do the trick. @abonislawski @jsarha
There was a problem hiding this comment.
Ok by me. @abonislawski , Ok if I just remove the channel_count == 0 check?
There was a problem hiding this comment.
No, we need to verify this somewhere, because later in the runtime code assumes that this check has been performed at this point, example:
sof/src/module/audio/sink_api.c
Line 113 in 3da554f
There was a problem hiding this comment.
Ok, I'll go back to the original.
There was a problem hiding this comment.
I guess the most correct place for these checks would be when the binding to pipelines are made, but since the probes is so far the only module that lives outside pipelines, I think this is adequate for now.
bd8f865 to
cf67fd4
Compare
The channel check introduced in commit:
cdb7f91 ("audio: module_adapter: bound host channels_count at ipc4 init")
does not let probes module through. Add extra condition to allows probes that relies on ipc4 probes implementation on Linux ASoC side, that initializes the whole base config as zero for probes module.
Fixes: #11254